fix(reactivity): toRef edge cases for ref unwrapping #12420
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
with the following steps: 1. switch to main branch
2. nr prebench-compare
3. nr bench ref
4. switch to this PR
5. nr prebench-compare
6. nr bench-compare refenvinfo: System:
OS: macOS 14.5
CPU: (8) arm64 Apple M1
Memory: 127.44 MB / 16.00 GB
Shell: 5.9 - /bin/zsh
Binaries:
Node: 22.7.0 - /usr/local/bin/node |
edison1105
added
scope: reactivity
need discussion
🍰 p2-nice-to-have
|
@edison1105 Would you be able to try that performance benchmark again? I tried running it locally, but it seems to get stuck running I don't think my changes should impact those benchmarks. The benchmark you mentioned seems to be testing I would expect some performance impact on |
|
I tested it again. This time, I repeated step 6 above three times, and the results showed that performance was not affected. Below are the results from the third execution. The above test results were obtained from the first time run ✓ packages/reactivity/__benchmarks__/ref.bench.ts (4) 16730ms
✓ ref (4) 16728ms
name hz min max mean p75 p99 p995 p999 rme samples
· create ref 21,076,099.66 0.0000 0.2064 0.0000 0.0000 0.0001 0.0001 0.0002 ±0.23% 10538050 [1.00x] ⇓
create ref 21,152,011.37 0.0000 0.5231 0.0000 0.0000 0.0001 0.0001 0.0002 ±0.38% 10576006 (baseline)
· write ref 20,557,804.48 0.0000 0.4285 0.0000 0.0000 0.0001 0.0001 0.0001 ±0.51% 10278903 [1.02x] ⇑
write ref 20,076,703.63 0.0000 0.5026 0.0000 0.0000 0.0001 0.0001 0.0002 ±0.52% 10038353 (baseline)
· read ref 24,080,925.98 0.0000 0.7341 0.0000 0.0000 0.0000 0.0001 0.0002 ±1.10% 12040464 [0.99x] ⇓
read ref 24,253,846.74 0.0000 1.1237 0.0000 0.0000 0.0000 0.0001 0.0001 ±1.02% 12126924 (baseline)
· write/read ref 19,876,380.25 0.0000 0.5281 0.0001 0.0000 0.0001 0.0001 0.0002 ±0.64% 9938191 [1.03x] ⇑
write/read ref 19,240,027.69 0.0000 2.3183 0.0001 0.0000 0.0001 0.0001 0.0002 ±1.21% 9620095 (baseline) |
edison1105
added
ready to merge
|
Warning Rate limit exceeded@edison1105 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes extend Vue's reactivity system with enhanced toRef behavior for nested refs and shallow contexts. Adds comprehensive test coverage for toRef edge cases including array handling, nested ref propagation, and shallow variant interactions. Exports Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant toRef
participant ObjectRefImpl
participant Proxy
participant Source
User->>toRef: toRef(source, key)
activate toRef
toRef->>ObjectRefImpl: new ObjectRefImpl(source, key, defaultValue)
activate ObjectRefImpl
ObjectRefImpl->>Source: Determine if shallow/raw
ObjectRefImpl-->>ObjectRefImpl: Store _raw, _shallow
deactivate ObjectRefImpl
toRef-->>User: Return Ref
deactivate toRef
User->>Ref: ref.value (get)
activate Ref
rect rgb(200, 230, 255)
note over Ref: Shallow unwrapping logic
alt _shallow flag is true
Ref->>Source: Get source[key]
Ref->>Ref: Unwrap nested proxy if present
Ref-->>User: Return unwrapped value
else _shallow flag is false
Ref->>Source: Get source[key]
Ref-->>User: Return raw value
end
end
deactivate Ref
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/ecosystem-ci run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/reactivity/__tests__/ref.spec.ts(3 hunks)packages/reactivity/src/baseHandlers.ts(2 hunks)packages/reactivity/src/ref.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: naramdash
Repo: vuejs/core PR: 14058
File: packages/reactivity/src/collectionHandlers.ts:252-271
Timestamp: 2025-11-05T14:53:01.052Z
Learning: In Vue's reactivity system, collection methods that create new collections (like Array's slice(), concat(), filter() and Set's difference(), intersection(), union(), symmetricDifference()) intentionally return raw (non-reactive) collections, not reactive proxies. This is by design - only the values/elements inside can remain reactive if they were originally reactive.
🧬 Code graph analysis (3)
packages/reactivity/__tests__/ref.spec.ts (2)
packages/reactivity/src/ref.ts (4)
ref(61-63)toRef(477-491)isRef(46-48)Ref(28-37)packages/reactivity/src/baseHandlers.ts (1)
get(55-134)
packages/reactivity/src/baseHandlers.ts (2)
packages/shared/src/general.ts (3)
isArray(39-39)isIntegerKey(77-81)hasOwn(34-37)packages/reactivity/src/reactive.ts (3)
isReadonly(336-338)isShallow(340-342)toRaw(378-381)
packages/reactivity/src/ref.ts (4)
packages/reactivity/src/reactive.ts (4)
toRaw(378-381)isProxy(351-353)isShallow(340-342)Target(18-24)packages/reactivity/src/index.ts (6)
toRaw(33-33)isProxy(29-29)isShallow(28-28)ReactiveFlags(82-82)unref(8-8)isRef(4-4)packages/shared/src/general.ts (1)
isIntegerKey(77-81)packages/reactivity/src/dep.ts (2)
Dep(67-205)getDepFromReactive(391-397)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
- GitHub Check: test / e2e-test
🔇 Additional comments (4)
packages/reactivity/src/baseHandlers.ts (1)
142-177: Array index writes now consistently treat elements as raw valuesGuarding the ref-unwrapping branch with
!isArrayWithIntegerKeyand reusingisArrayWithIntegerKeyforhadKeymakes array integer index writes behave like plain value replacement (not “mutate nested ref in-place”), matching the existing get-trap behavior that does not unwrap refs for numeric array indices. This keeps object properties with refs using the nested-ref update path while letting array indices be managed explicitly via their refs or viatoRef’s newObjectRefImpllogic. Looks correct and aligns array semantics with the broader collection design.packages/reactivity/__tests__/ref.spec.ts (1)
312-337: New toRef tests comprehensively cover non-reactive, shallow, readonly, and array edge casesThe added suites around
toRef(plain objects with nested refs + defaults, array indices/custom props, lazy computed evaluation through proxies, shallowReactive/shallowReadonly combos, and readonly-wrapped shallow/regular proxies) tightly exercise the newObjectRefImpllogic and the array handler tweaks. The scenarios match the PR’s listed edge cases and should catch regressions in ref unwrapping and proxy-bypass behavior.Also applies to: 339-389, 414-555
packages/reactivity/src/ref.ts (2)
352-402: ObjectRefImpl’s_shallowdetection and setter routing match the proxy/unwrapping modelThe new
_raw+_shallowlogic inObjectRefImplcleanly separates when the object-ref itself should handle unwrapping/nested-ref writes vs. when to defer entirely to the underlying proxies:
- Walking the proxy chain via
RAWand using!isProxy(obj) || isShallow(obj)means any non‑shallow proxy (e.g.reactive,readonly) flips_shallowtofalse, soget valuejust returns what the proxy gives andset valuegoes through the proxy, preserving readonly semantics.- Arrays with integer keys are explicitly exempted from that walk, keeping
_shallowtruesoget/setcan compensate for the array handlers’ “do not unwrap element refs” rule and provide the expectedtoRef(array, index)behavior.- The setter’s
isRef(this._raw[this._key])guard ensures nested-ref forwarding only happens while the raw slot actually holds a ref; once the slot is replaced (e.g. set toundefined), subsequent writes correctly fall back to normal proxyset.- The
depgetter delegation togetDepFromReactive(this._raw, this._key)nicely reuses the existing dependency graph sotriggerRefworks with property refs without duplicating tracking.Taken together, this lines up with the new tests for shallow/reactive/readonly combinations and the array index cases.
493-499:propertyToRefno longer returns the existing nested ref instance
propertyToRefnow always returns a freshObjectRefImplinstead of returningsource[key]when it happens to be a ref. This is an intentional behavioral change:
- It fixes the lazy-evaluation issue for computed props on reactive objects, because
toRefno longer has to read the property at creation time.- It makes
toRefsemantics uniform across plain/reactive/shallow/readonly sources and arrays, at the cost of no longer preserving object identity (toRef(obj, 'x') !== obj.xeven whenobj.xis already a ref).Given the updated tests no longer rely on identity and explicitly cover the new behavior, this change looks deliberate and consistent; just worth keeping in mind as a subtle observable change for library consumers that previously depended on
===checks.
|
📝 Ran ecosystem CI: Open
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
3 participants
This PR aims to address several related edge cases.
Case 1
The original motivation came from vuejs/pinia#2812.
Long story short, consider the following code:
The call to
toRefcurrently triggers the logging, even if the returned ref is never used:While attempting to fix this, I kept running into other edge cases, mostly involving
toRefandshallowReactive/shallowReadonly. I've tried to fix all of those edge cases too.Case 2
Consider the following:
This should result in
t.valuebeing1, but currently it ends up as0.Case 3
Consider this example:
t.valueshould be7, respecting the default value. But currently it isundefined.Case 4
This is somewhat separate, but it impacted the tests I wrote for array handling with
toRef.Consider the following:
Arrays don't unwrap refs used as elements, but they do unwrap arrays added using custom properties such as
foo. Essentially, custom properties on reactive arrays behave just like properties of normal reactive objects. Apart from in the case outlined above, where the ref is replaced rather than updated byarr.foo = 2. I've changed this inbaseHandlers.ts, so that updating a custom property on an array behaves the same as for an object:Notes on the changes
The key change is to move the logic for handling nested refs from
propertyToRefto insideObjectRefImpl.Detecting whether a source will automatically unwrap refs is a bit tricky. It isn't sufficient just to check for
isShallow, as you could haveshallowReadonlywrapped aroundreactive. Instead I've recursively checked each wrapper proxy to check whether any level would unwrap.As noted earlier, arrays also pose a specific challenge, as they don't unwrap elements even inside
reactiveorreadonly.There is one existing test case that no longer passes. Specifically, this one:
This is expecting the exact same ref to be returned by
toRef. But returning the same ref is the underlying cause of most of the edge cases outlined above. From what I can tell, the original motivation for that test case was just to ensure that nested refs are unwrapped, not that they need to be===. Returning an equivalent ref should be sufficient.I do wonder whether there's an easier way to implement all of this, but currently I'm not seeing it.
Summary by CodeRabbit
Release Notes
New Features
shallowReadonlyAPI for creating shallow readonly reactive proxies.Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.